-
Notifications
You must be signed in to change notification settings - Fork 898
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to control trailing zero in floating-point literals #5834
base: master
Are you sure you want to change the base?
Conversation
Hey @amatveiakin I know this hasn't gotten much attention from the team, but I wanted to check back in and see if you're still interested in working on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wrapped up my initial review on this. Overall I think this is a good start, but I'd like us to expand on the test cases.
return wrap_str( | ||
context.snippet(span).to_owned(), | ||
context.config.max_width(), | ||
shape, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Version::Two
we don't check the line length of string literals because there's no meaningful way to break them by default. Similarity, I don't think there would be a meaningful way to break a float literal over multiple lines so let's avoid using wrap_str
. Same goes for the wrap_str
call below.
For reference, here's the code in rewrite_string_lit
that I'm referring to:
Lines 1261 to 1263 in cedb7b5
&& context.config.version() == Version::Two | |
{ | |
return Some(string_lit.to_owned()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please elaborate? I guess the statement “there is no meaningful way to break a float literal over multiple lines” is always true, so why would this depend on config version?
BTW, would you say that rewrite_int_lit
should be updated as well? It also calls wrap_str
unconditionally:
Lines 1306 to 1310 in cedb7b5
wrap_str( | |
context.snippet(span).to_owned(), | |
context.config.max_width(), | |
shape, | |
) |
fn float_literals() { | ||
let a = 0.; | ||
let b = 0.0; | ||
let c = 100.; | ||
let d = 100.0; | ||
let e = 5e3; | ||
let f = 5.0e3; | ||
let g = 7f32; | ||
let h = 7.0f32; | ||
let i = 9e3f32; | ||
let j = 9.0e3f32; | ||
let k = 1000.00; | ||
let l = 1_000_.; | ||
let m = 1_000_.000_000; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start, but I'd really like to see us expand on these test cases.
- Can we move all
float_literal_trailing_zero
tests to atests/{source|target}/config/float_literal_trailing_zero
directory. Then you can name each test file as the name of the enum variant. - To cover all our bases let's add a test case for
Preserve
. We should only need atarget
test file for the preserve case since we don't expect formatting to change. - Let's try to be as thorough as we can when coming up with test cases. You've already highlighted let bindings, but there are plenty of other places where float literals can be written. A few that come to mind are conditional expressions, struct literals, match patterns, function arguments, array literals, macro calls, etc.
One thing that I'm particularly interested in seeing is what happens when we're at or near the max_width
boundary. Does adding the extra 0
correctly wrap a function call or array literal when adding the extra 0
pushes us over the max_width
, and do we correctly format on a single line when adding 0
pushes us to exactly the max_width
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please tell me if I did the tests right. Three separate test files seem like a lot for such a small feature, but I don't see how else I could check all configurations.
Oh and to answer your question, this is correct. You'll need to create individual test files for each case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Done.
- Done.
- I've added one macro and one non-macro example that causes a line wrapping change. Do you think we really need to cover all these cases (conditional expressions, struct literals, match patterns, function arguments, etc.)? To me it feel a bit excessive. And it seems different from the current testing strategy (take
hex_literal_case
tests as a close example).
|
||
- **Default value**: `Preserve` | ||
- **Possible values**: `Preserve`, `Always`, `IfNoPostfix`, `Never` | ||
- **Stable**: No (tracking issue: [#3187](https://github.com/rust-lang/rustfmt/issues/3187)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we'll need to add a new tracking issue for this one. #3187 isn't a tracking issue. We can certainly add the tracking issue after this PR is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't we create an issue now, so that we don't have to go back and modify the link later?
Is there a document that describes how a tracking issue should look like?
Apologies for the slow reply. Please take another look. |
@amatveiakin No worries. I'll likely have some time to get back to this later this week. For your awareness I want to let you know that I just merged #6085, and I'd really like to see tests with range expressions like the ones in the PR. I want to make sure that the new |
This is my first contribution to rustfmt. Feel free to nitpick :)
Also please tell me if I did the tests right. Three separate test files seem like a lot for such a small feature, but I don't see how else I could check all configurations.
Closes #3187